Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Permission check fixed for the serviceaccount of the target allocator #3391

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Permission check fixed for the serviceaccount of the target allocator #3391

merged 2 commits into from
Oct 28, 2024

Conversation

davidhaja
Copy link
Contributor

Description:
Permission check fixed for the serviceaccount of the target allocator.
If the opentelemetrycollector.spec.targetAllocator.serviceAccount is not set, the operator
checks the system:serviceaccount:default:<EMPTY SA NAME> serviceaccount instead of the generated serviceaccount name.

Warning: missing the following rules for networking.k8s.io/ingresses: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nodes: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nodes/metrics: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for endpoints: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for pods: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for discovery.k8s.io/endpointslices: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nonResourceURL: /metrics: [get,system:serviceaccount:default:]
Warning: missing the following rules for monitoring.coreos.com/servicemonitors: [*,system:serviceaccount:default:]
Warning: missing the following rules for monitoring.coreos.com/podmonitors: [*,system:serviceaccount:default:]
Warning: missing the following rules for services: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for namespaces: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for configmaps: [get,system:serviceaccount:default:]

This PR resolves this issue using the target allocator's serviceaccount naming logic.

Link to tracking Issue(s): 3380

Testing:

Documentation:

@davidhaja davidhaja requested a review from a team as a code owner October 25, 2024 06:49
@iblancasa iblancasa self-requested a review October 25, 2024 06:53
@iblancasa
Copy link
Contributor

checks the system:serviceaccount:default: serviceaccount instead of the generated serviceaccount name.

Is that the default service account the TA will use?

@davidhaja
Copy link
Contributor Author

checks the system:serviceaccount:default: serviceaccount instead of the generated serviceaccount name.

Is that the default service account the TA will use?

Yes.

swiatekm
swiatekm approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! I'd love to see a test verifying that we call the function correctly. Changing the order of two string arguments in a function call is an easy mistake to make.

@jaronoff97
Copy link
Contributor

+1 to Mikolaj's suggestion though :)

@davidhaja
Copy link
Contributor Author

Thanks for fixing this! I'd love to see a test verifying that we call the function correctly. Changing the order of two string arguments in a function call is an easy mistake to make.

I'm wondering if it makes sense to include the serviceaccount name in the warning message?
So like:

Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nonResourceURL: /metrics: [get]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - monitoring.coreos.com/podmonitors: [*]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nodes/metrics: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - pods: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - namespaces: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - configmaps: [get]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - networking.k8s.io/ingresses: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - monitoring.coreos.com/servicemonitors: [*]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nodes: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - services: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - endpoints: [get,list,watch]

@swiatekm @jaronoff97 wdyt?

@davidhaja
Copy link
Contributor Author

Thanks for fixing this! I'd love to see a test verifying that we call the function correctly. Changing the order of two string arguments in a function call is an easy mistake to make.

I've included the serviceaccount information in the warning messages (system:serviceaccount:<namespace>:<serviceaccount name>)
Also modified the tests that cover the warning messages with this information.
So with this change the tests cover if the namespace and serviceaccount name is switched during the execution, because they are coming from the opentelemetrycollector and targetallocator CRs and included in the warning log.
@swiatekm @jaronoff97 Let me know if this modification satisfies your request, or if I need to add a separate test for this purpose.

@swiatekm swiatekm self-requested a review October 28, 2024 16:26
@jaronoff97 jaronoff97 merged commit afc0cae into open-telemetry:main Oct 28, 2024
36 checks passed
@alexkruc alexkruc mentioned this pull request Dec 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@jaronoff97 jaronoff97 jaronoff97 approved these changes

@iblancasa iblancasa iblancasa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RBAC warnings when using custom service account/rbac
4 participants
@davidhaja @iblancasa @jaronoff97 @swiatekm